Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override default_run_env instead of base_env #75

Merged
merged 13 commits into from
Aug 23, 2024
Merged

Override default_run_env instead of base_env #75

merged 13 commits into from
Aug 23, 2024

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Aug 22, 2024

I swear I fixed it before, but apparent I didn't.

In Kedro, the two environments work as:

  • base_env: base
  • default_run_env: local.

If user is changing to prod environment, it should be base + prod instead of prod + local (current status).
For example, kedro run --env=prod is equivalent to base_env='base', run_env='prod'

The starting status bar now look like this:
image

Dev Notes

  • I also updated the command to indicate that Select Kedro Run environment instead of base
  • The default is now change to base + local. It's not possible to change base environment via the extension. This is consistent to how Kedro CLI works too. It's rare one need to change base_env.
  • User won't be able to choose base, as otherwise it will become base + base. We discussed briefly there may be user want to use a single environment, but we are not sure if this requirement exists, so we didn't implement this and will wait for FR instead.

Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
@noklam noklam requested review from jitu5 and removed request for astrojuanlu and jitu5 August 22, 2024 13:00
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understood correctly, now the base environment will always be base, and the action/button changes the run environment, right? And the default would be local?

This makes more sense 👍🏼 Would the user still have the option to set the run env to base?

@noklam
Copy link
Contributor Author

noklam commented Aug 22, 2024

So if I understood correctly, now the base environment will always be base, and the action/button changes the run environment, right? And the default would be local?

Correct, it should be the same as Kedro default.

This makes more sense 👍🏼 Would the user still have the option to set the run env to base?

hm... good point, should we hide base from the options to avoid confusion? Technically you can change your "base_env" via settings.py, but I think that's a niche case that we can ignore from the extension.

On the other hand, users barely use local so they put everything in base, if they saw local as default I don't know if that's confusing. base + local may not be better either.

image

Signed-off-by: Nok Lam Chan <[email protected]>
@astrojuanlu
Copy link
Member

I think base + local would be better actually. And maybe we leave the base option and so the text becomes just base?

@noklam
Copy link
Contributor Author

noklam commented Aug 23, 2024

juanlu
default should be whatever settings.py says (by default, base + local with our typical starters, right?)
if users switch to prod, I'd expect base + prod
switching to base is weird 😅 but maybe somebody wants to disable the env completely?? I don't know
if we do keep the option to switch to base, then the IDE should display base (rather than base + base, that looks silly)
I'm +1 on removing base from the env selection, to miminise confusion

For the record we agree on above in private discussion.

@noklam noklam merged commit 7b5b2a8 into main Aug 23, 2024
2 checks passed
@noklam noklam deleted the noklam/bug-env branch August 23, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants